Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/prometheusexporter Shutdown HTTP server #35465

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Argannor
Copy link

@Argannor Argannor commented Sep 27, 2024

Description: Shutdown the http.Server instance on exporter shutdown

Link to tracking Issue: #35464

Testing: Manual testing. I included this version of the exporter in our internal distribution and deployed it to verify the metrics no longer go stale after receiving a NOHUP signal.

Documentation: n/a

@Argannor Argannor requested review from dashpole and a team as code owners September 27, 2024 14:57
Copy link

linux-foundation-easycla bot commented Sep 27, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@Argannor Argannor force-pushed the fix/prometheusexporter-shutdown-server branch from 00e689f to 52eee00 Compare September 27, 2024 15:03
@Argannor
Copy link
Author

Hey @Aneurysm9, @songy23, sorry for pinging you directly. Do you mind having a look at this or advising me what is needed from me to go ahead with this PR?

@Argannor Argannor force-pushed the fix/prometheusexporter-shutdown-server branch from 1c1ef8f to 6efa063 Compare October 11, 2024 13:59
@dashpole
Copy link
Contributor

unit tests seem to fail after the change.

@Argannor
Copy link
Author

Yeah, I just took a look at the teste that are failing, and I'm not sure what's happening there:

	t.Cleanup(func() {
		require.NoError(t, exp.Shutdown(context.Background()))
		// trigger a get so that the server cleans up our keepalive socket
		var resp *http.Response
		resp, err = httpClient.Get("https://localhost:7777/metrics")
		require.NoError(t, err)
		require.NoError(t, resp.Body.Close())
	})

Is this a workaround for the fact that the server did not shutdown correctly previously, and is now failing, because after the shutdown the server is shutdown?

@dashpole
Copy link
Contributor

It might have been. Try removing the httpClient.Get call from cleanup and see if tests pass.

These statements seem to be a workaround for the server not being closed correctly, with the previous changes this should no longer be necessary
@Argannor Argannor force-pushed the fix/prometheusexporter-shutdown-server branch from 381f74f to 7b7535f Compare October 14, 2024 09:26
@Argannor
Copy link
Author

After removing the statements the tests passed locally for me. Sorry for not running them in the first place, I had some troubles running them using make... :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants